-
Notifications
You must be signed in to change notification settings - Fork 914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW] Clip decimal binary op precision at max precision #8194
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.06 #8194 +/- ##
===============================================
Coverage ? 82.84%
===============================================
Files ? 105
Lines ? 17866
Branches ? 0
===============================================
Hits ? 14801
Misses ? 3065
Partials ? 0 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add to the docstring of _binop_precision
mentioning that this helper function does not guarantee result to be bounded by cudf.Decimal64Dtype.MAX_PRECISION
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but have a question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the back and forth. Just a small comment reverting my previous review.
Does this PR close an issue, and if so can you reference it here? |
Nope, there's no issue related to this problem |
@ChrisJar In that case is there more context we can add to this PR? Adding some tests that would have previously failed, and/or providing the error that was being encountered previously could prove useful if we end up with a regression later on. |
Yep! Will do |
Thanks to @cwharris 's question that prompted me to think deeper: since we compute the precisions pessimistically, when clipping happens, it means the operation might have produced an overflow. Do we need to introspect the data to tell if the overflow has happened? What is current behavior when overflow happens? |
Yep good point. When the underlying int value of the result exceeds |
So are changes needed on this PR, or is it ready to merge? |
@harrism Yep! It's ready to merge |
@gpucibot merge |
This fixes an issue where the precision assigned to the result of a decimal binary operation may exceed the maximum precision.
Closes #8291